Begin enabling pre-commit hooks #2242

droctothorpe · 2023-11-08T02:16:18Z

What this PR does / why we need it:
This PR implements some basic pre-commit hooks and applies them retroactively to the entire codebase. All of the changes in this PR, other than the addition of .pre-commit-config.yaml, were automatically generated by the isort pre-commit hook. This pre-commit configuration is based off of Kubeflow Pipeline's to promote consistent code across the various Kubeflow repositories. The only difference is that I commented out the majority of the hooks, because if we tried to implement all of them simultaneously, the resulting PR would be enormous in size. This PR is a kind of proof of concept. If merged, I'll just progressively uncomment and enable the other pre-commit hooks (either sequentially or all once depending on preference) and execute them retroactively and submit one or more follow up PRs.

Not sure how to integrate it into katib's CI if anyone has any input on that. Highly recommend doing so. It's the only way to enforce the standards that pre-commit enables.

Which issue(s) this PR fixes
Fixes # #2187

Checklist:

Docs included if any changes are user facing

github-actions · 2024-02-06T05:05:10Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tenzen-y · 2024-02-12T15:00:39Z

/remove-lifecycle stale

tenzen-y · 2024-04-19T15:25:38Z

@droctothorpe Hey Alex! Sorry for the late response. I'm starting to review this PR. Are you still here?

droctothorpe · 2024-04-19T15:42:04Z

No worries! Happy to rebase and re-run if it's worth merging.

tenzen-y · 2024-04-19T15:45:38Z

No worries! Happy to rebase and re-run if it's worth merging.

Thank you!
I think that this would be worth it :)

droctothorpe · 2024-05-07T01:38:52Z

Apologies for the delay, @tenzen-y. Rebased and re-ran the preliminary pre-commit hooks. I also updated the Lint Files CI gate to run pre-commit and confirmed it works. Once this is merged, I'll submit follow up PRs that unlock additional hooks and further standardize the codebase.

droctothorpe · 2024-06-03T15:23:02Z

Bump cc @andreyvelich

andreyvelich

Thank you for adding this and sorry for the late review @droctothorpe!
I left a few comments.

.github/workflows/test-lint.yaml

.pre-commit-config.yaml

andreyvelich · 2024-06-12T16:12:56Z

pkg/apis/manager/health/python/health_pb2_grpc.py

@@ -1,6 +1,5 @@
 # Generated by the gRPC Python protocol compiler plugin. DO NOT EDIT!
 import grpc
-


Should we exclude files that auto-generated ?

I think it's okay to modify them, but am happy to update the pre-commit file if excluding them is preferable. There are quite a few files that fall into this category (assuming there aren't more that don't include the DO NOT EDIT warning:

❯ grep -r "DO NOT EDIT" . | wc -l 110

It looks like pre-commit doesn't let you exclude files based on content, just a regex for the filename, but the generated files have names that truly run the gamut. Given that these specific modifications are superficial and will be overwritten when the files are regenerated anyway, do you mind if we keep them?

It might be problematic, since we run make generate command to make sure user runs this script before making any API changes.
Can't we use exclude parameter ?
https://pre-commit.com/#top_level-exclude

We are doing it here: https://github.com/kubeflow/katib/blob/master/.github/workflows/test-go.yaml#L34

For example, I can see that KFP does it: https://github.com/kubeflow/pipelines/blob/master/.pre-commit-config.yaml#L48

Thanks, @andreyvelich! Any objection to following KFP's example and just listing the full paths? With 113 unique generated files, it's going to take forever to generate the 30 or so regexes that will be required to cover them all and will arguably reduce readability (regex is a write-only language 😂 ).

I guess, we don't need to list all 113 unique files, just find common patterns.
E.g.

exclude: (/pkg/client/|/pkg/mock/|/pkg/apis/manager|*zz_generated.deepcopy.go|sdk/python/v1beta1/kubeflow/katib/models/) WDYT @droctothorpe ?

You're absolutely, right! Sorry, I was being lazy. Fix incoming.

Done! Also squashed and removed all the generated files, then validated that it's excluding correctly by running pre-commit run --all-files and confirming that none of the generated files get modified.

sdk/python/v1beta1/kubeflow/katib/models/v1beta1_trial.py

droctothorpe · 2024-06-14T21:00:58Z

Thank you for adding this and sorry for the late review @droctothorpe! I left a few comments.

Thanks for the review, @andreyvelich!

.pre-commit-config.yaml

andreyvelich · 2024-07-12T21:48:20Z

Sorry for the late reply @droctothorpe!
Should we just move forward with this PR ? I think, we have just a few comments that we need to address.

droctothorpe · 2024-07-12T21:55:34Z

Will address ASAP. Thanks, @andreyvelich.

Signed-off-by: droctothorpe <[email protected]>

andreyvelich

Thanks a lot for updating this @droctothorpe!
Just a few small comments from me.
Please can you update the developer guide to explain how to install and run pre-commit.

Maybe we could add similar section as for Training Operator code style: https://github.com/kubeflow/training-operator/blob/master/docs/development/developer_guide.md#code-style

.pre-commit-config.yaml

pkg/apis/manager/health/python/health_pb2.pyi

pkg/apis/manager/v1beta1/python/api_pb2.pyi

Signed-off-by: droctothorpe <[email protected]>

andreyvelich

Thank you for this great contribution @droctothorpe!
If you have some bandwidth it would be great to add the same for Training Operator.
/lgtm
/assign @johnugeorge @tenzen-y

droctothorpe · 2024-07-16T13:16:13Z

Happy to set this up for the training operator as well and plan to submit follow up PRs to both repos to enable additional hooks.

andreyvelich

I think, we should be ready to merge this.
@droctothorpe Please can you create tracking issue to enable pre-commit for Training Operator ?
/approve

google-oss-prow · 2024-07-18T17:04:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Begin enabling pre-commit hooks Signed-off-by: droctothorpe <[email protected]> * Address PR feedback Signed-off-by: droctothorpe <[email protected]> --------- Signed-off-by: droctothorpe <[email protected]>

google-oss-prow bot added the size/XL label Nov 8, 2023

google-oss-prow bot requested review from andreyvelich, anencore94 and johnugeorge November 8, 2023 02:16

github-actions bot added the lifecycle/stale label Feb 6, 2024

google-oss-prow bot removed the lifecycle/stale label Feb 12, 2024

droctothorpe force-pushed the pre-commit branch 5 times, most recently from 00d9930 to fa0552a Compare May 7, 2024 01:36

andreyvelich reviewed Jun 12, 2024

View reviewed changes

droctothorpe force-pushed the pre-commit branch from fa0552a to fa04b5b Compare June 14, 2024 20:59

andreyvelich reviewed Jun 15, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

droctothorpe force-pushed the pre-commit branch 2 times, most recently from 34e5411 to 3ad430f Compare July 15, 2024 14:15

Begin enabling pre-commit hooks

8c826c3

Signed-off-by: droctothorpe <[email protected]>

droctothorpe force-pushed the pre-commit branch from 3ad430f to 8c826c3 Compare July 15, 2024 19:05

google-oss-prow bot added size/L and removed size/XL labels Jul 15, 2024

andreyvelich reviewed Jul 15, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

pkg/apis/manager/health/python/health_pb2.pyi Outdated Show resolved Hide resolved

pkg/apis/manager/v1beta1/python/api_pb2.pyi Outdated Show resolved Hide resolved

droctothorpe force-pushed the pre-commit branch from 3515fee to bae5428 Compare July 15, 2024 22:54

Address PR feedback

5be9e47

Signed-off-by: droctothorpe <[email protected]>

droctothorpe force-pushed the pre-commit branch from bae5428 to 5be9e47 Compare July 15, 2024 22:55

andreyvelich reviewed Jul 16, 2024

View reviewed changes

google-oss-prow bot assigned johnugeorge, tenzen-y and andreyvelich Jul 16, 2024

google-oss-prow bot added the lgtm label Jul 16, 2024

andreyvelich reviewed Jul 18, 2024

View reviewed changes

google-oss-prow bot added the approved label Jul 18, 2024

google-oss-prow bot merged commit a3dd708 into kubeflow:master Jul 18, 2024
60 checks passed

droctothorpe mentioned this pull request Jul 18, 2024

Enable pre-commit for repo kubeflow/training-operator#2178

Closed

droctothorpe deleted the pre-commit branch July 19, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Begin enabling pre-commit hooks #2242

Begin enabling pre-commit hooks #2242

droctothorpe commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Feb 6, 2024

tenzen-y commented Feb 12, 2024

tenzen-y commented Apr 19, 2024

droctothorpe commented Apr 19, 2024

tenzen-y commented Apr 19, 2024 •

edited

Loading

droctothorpe commented May 7, 2024 •

edited

Loading

droctothorpe commented Jun 3, 2024

andreyvelich left a comment

andreyvelich Jun 12, 2024

droctothorpe Jun 14, 2024

droctothorpe Jul 15, 2024

andreyvelich Jul 15, 2024

andreyvelich Jul 15, 2024

andreyvelich Jul 15, 2024

droctothorpe Jul 15, 2024

andreyvelich Jul 15, 2024 •

edited

Loading

droctothorpe Jul 15, 2024 •

edited

Loading

droctothorpe Jul 15, 2024

droctothorpe commented Jun 14, 2024

andreyvelich commented Jul 12, 2024

droctothorpe commented Jul 12, 2024

andreyvelich left a comment

andreyvelich left a comment

droctothorpe commented Jul 16, 2024

andreyvelich left a comment

google-oss-prow bot commented Jul 18, 2024

		@@ -1,6 +1,5 @@
		# Generated by the gRPC Python protocol compiler plugin. DO NOT EDIT!
		import grpc

Begin enabling pre-commit hooks #2242

Begin enabling pre-commit hooks #2242

Conversation

droctothorpe commented Nov 8, 2023 • edited Loading

github-actions bot commented Feb 6, 2024

tenzen-y commented Feb 12, 2024

tenzen-y commented Apr 19, 2024

droctothorpe commented Apr 19, 2024

tenzen-y commented Apr 19, 2024 • edited Loading

droctothorpe commented May 7, 2024 • edited Loading

droctothorpe commented Jun 3, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

droctothorpe Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droctothorpe commented Jun 14, 2024

andreyvelich commented Jul 12, 2024

droctothorpe commented Jul 12, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

andreyvelich left a comment

Choose a reason for hiding this comment

droctothorpe commented Jul 16, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jul 18, 2024

droctothorpe commented Nov 8, 2023 •

edited

Loading

tenzen-y commented Apr 19, 2024 •

edited

Loading

droctothorpe commented May 7, 2024 •

edited

Loading

andreyvelich Jul 15, 2024 •

edited

Loading

droctothorpe Jul 15, 2024 •

edited

Loading